reland(#145): destination-table guard (--overwrite) + honest extension handling#149
Conversation
…ension handling (#145) Two fixes for the same failure class — the customer uploads their whole dataset and only then learns it was doomed: cli#70 (P4-lite) — table-exists guard: - One cheap read (the data list query) after cluster discovery, BEFORE staging. Existing table without --overwrite → exit 6 (new, documented) with the full remedy; --overwrite replaces it via the exact teardown data delete uses. The check fails OPEN with a visible note (the in-cluster duplicate check still backstops). - Teardown acts on the MATCHED name, not the flag's casing (Linux MySQL + PVC paths are case-sensitive; acting on the flag spelling could silently no-op the DROP/rm and claim success). - --overwrite + --idempotency-key is refused outright: a replayed submit attaches to the PREVIOUS run after the teardown deleted the data — false success + data loss. (Adversarial-review catch.) - Honest partial-failure copy: a half-finished replace names `data delete` as the primary recovery — a plain re-run would pass the DB-backed guard and hit the leftover files after a full upload. - The teardown pod honors --stage-pod-image (air-gapped registries). cli#68 — extension detection/emission: - .webp removed from the accept-set: the ingestor's FileExtension enum + the ingest.v1 schema allow only .jpg/.jpeg/.png for images, and FileTypeValidator RAISES on webp — accepting it locally guaranteed an in-cluster failure after the full upload. (The old comment claiming chart support was itself the cli#68 drift.) - The single shared extension is detected, shown in the summary ("3 files (.png)"), and emitted as spec.file_options.extension so the cluster validates the type that was actually staged — previously it checked its .jpeg convention default and rejected .jpg/.png datasets after upload. - Mixed types fail locally with counts (exit 3); an all-unsupported dataset names what was found vs accepted. Cross-repo traced against data-ingestors (conventions merge, per- category validator factories, DuplicateValidator) and live-verified on a real cluster: PNG detection, guard on an existing table (exit 6), --overwrite dry-run creates nothing, mixed extensions refused, combo flag refusal. go build/vet/test green; new tests cover the guard seam (matched-name contract, fail-open), extension detection, spec emission + schema validation (keypoint top-level fields pinned), and the summary rendering. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
👋 Heads-up — Code review queue is at 37 / 30 Above the WIP limit. The team convention is to review existing PRs before opening new work. Open PRs currently in Code review (oldest first):
Pull from review before opening new work. (This is a nudge from the kanban WIP check, not a block.) |
|
@saadqbal heads-up on this morning's stack: #142 merged to develop at 07:16 ✓, but #145 (08:47) merged into #142's already-merged branch, and #147 (08:50) into #145's — so both show merged while develop contains neither (verified: Fix, ready to go: merge #149 (this, = #145's exact approved content on develop, byte-identical diff), then #150 (= #147's content, auto-retargets here). Both suites green. Nothing else needed — and no content was lost. For future stacks: merge bottom-up before the base merges, or retarget the child to |
… checked contract (#150) * feat(data ingest): preflight parity — the dry-run's promise becomes a checked contract backend#828 P3; closes cli#69, cli#71, cli#72, cli#73. Every local check now previews a NAMED data-ingestors validator with matching semantics, so 'preflight passed' means the in-cluster validation passes too — failures land BEFORE the upload, not after. New previews (internal/push/preflight.go, each cites its source rule): - label column exists (LabelColumnValidator: exact, then case-insensitive+trimmed — never stricter than the cluster) (#69) - BOM: tabular rejected pre-upload (the in-cluster stdlib schema probe falsely rejects BOM'd CSVs — data-ingestors#338); image/text BOM accepted+stripped, matching the pandas paths (#71) - every image decoded (header-only, cheap): zero-byte, corrupt, resolution vs target — plus the labels↔images cross-check with the ingestor's _has_extension naming semantics (dotted stems!) (#72) - duplicate headers (stripped, case-SENSITIVE like the probe), zero data rows, --schema columns ⊆ header, CSV encoding gate (check_csv_encoding preview: UTF-8 + no NUL) (#73 + gaps found) - label diversity (LabelDiversityValidator: >=2 classes; NA-sentinel drop + numeric collapse for schema-typed tabular labels; empty string IS a class for image/text) — discovered BY the harness's first run, was in no ticket - object_detection images↔annotations stem pairing (FilePairingValidator preview) FIXES A PRE-EXISTING SHIP-BLOCKER found by the adversarial pass: spec.go swapped target_size to [H,W] on emit (mistaken review note) — but the schema + ImageResolutionValidator compare PIL's (W,H) verbatim, so EVERY non-square dataset failed in-cluster post-upload. Emission is now [W,H]; the parity pair imgc-nonsquare / imgc-nonsquare-swapped pins the orientation end-to-end against the real validator. THE PARITY HARNESS (the durable part): - internal/push/testdata/parity: 23 fixture cases + goldens.json GENERATED from the real Python validators (scripts/gen-validator-goldens.py; scripts/sync-validator-goldens.sh --check for drift, verdict-level) - parity_golden_test.go asserts the PRODUCTION dispatch (push.PreflightDataset — shared by runDataIngest and the test, so the two cannot drift) reaches the manifest's verdict per case, and that committed goldens match the manifest — an ingestor rule change fails the test until consciously reconciled - deliberate divergences (read-/transfer-time failures the ingestor's preflight can't see but the CLI previews) are explicit manifest notes, never silent Verified: 23/23 parity green; full go test green; live dry-runs on a real cluster (missing file, bad label column, single-class, latin-1, non-square 320x200 accept). Adversarial review (2 lenses, high effort): all findings folded incl. the [W,H] bug, value-semantics divergences in diversity/cross-check, the encoding gate, de-masked fixtures, the shared dispatch, and a vacuous kubeconfig test revived with decodable fixtures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(preflight): label diversity respects the label's SQL type (#152) CheckLabelDiversity collapsed numeric-looking labels ("1"/"1.0") for every tabular_classification dataset. The in-cluster LabelDiversityValidator pins dtype=str for string-family schema types (VARCHAR/CHAR/TEXT/STRING), so those labels stay distinct — only numeric types get pandas numeric inference (data-ingestors #252). A user-declared VARCHAR label with numeric-looking classes was wrongly rejected at preflight. Derive the drop-NA and collapse-numeric flags from the label's declared schema type at the dispatch site; keep image/text (untyped) unchanged. Adds leaf + dispatch tests. This aligns the Go side with the golden generator, which already types columns as VARCHAR. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Asad Iqbal (Saadi) <asad.dsoft@gmail.com>
|
Consolidated the reland here so this is the single PR that lands everything on
|
preflight.go used bare `defer f.Close()`, which the required Lint job's
`errcheck ./...` rejects on develop. Match the repo convention used in
detect.go / tabular.go / stream.go: `defer func() { _ = f.Close() }()`.
These are read-only opens for validation, so dropping the close error is
intentional. Slipped through earlier because the stacked merges never hit
the full Lint-on-develop gate.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…] emit (#153) The TargetSize field NOTE still described the old #25/[H,W] behavior ("EMITTED as [height, width] ... buildImage does the swap"), which #147 (landed via #149) deliberately reverted. buildImage now emits [TargetSize[0], TargetSize[1]] = [width, height] with no swap — the order ingest.v1.json documents ("matches PIL.Image.size and what ImageResolutionValidator expects") and the order image_validator.py compares against verbatim. Rewrite the NOTE to state stored-and-emitted [width, height], no swap, and warn against re-introducing the swap (which failed every non-square dataset). Comment-only; rolls up under #145/#147. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Why a re-land
#145 was approved + merged at 08:47 — but into its stacked base
ux/cli-surface-and-namespace-discovery, whose own PR (#142) had already merged todevelopat 07:16. So #145 shows merged while its content never reached develop (verified:--overwriteabsent fromorigin/develop).This PR cherry-picks #145's squash (
090be58) onto current develop — byte-identical to the approved content (git diffagainst the merged branch is empty), fullgo test ./...green on top of develop.No re-review needed beyond a sanity glance — it's the exact code @saadqbal already approved in #145. Merging THIS one to develop completes what #145 intended.
(#147 has been rebased onto this branch and retargeted — the stack is consistent again. For future stacked merges: retarget to develop first, or merge bottom-up before the base branch merges.)
🤖 Generated with Claude Code